Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(kitchen-sink): update dependencies and fix a key issue in the todo list #136

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

theborowski
Copy link
Contributor

@theborowski theborowski commented Dec 17, 2024

changes

  • bump react and react-dom from the RC version to the official 19.0.0 release
  • remove unused babel-plugin-react-compiler dependency
  • move vite related dependencies into devDependencies
  • fix a tiny bug where adding two of the same text items in the list would result in a key error
  • prevent adding an item to the list with the enter key if it's text is blank

visual diff

Screen.Recording.2024-12-17.at.8.09.28.PM.mov

Copy link

vercel bot commented Dec 17, 2024

@theborowski is attempting to deploy a commit to the Million Team on Vercel.

A member of the Team first needs to authorize it.

{tasks.map((task) => (
<TaskItem key={task} task={task} onDelete={onDelete} />
{tasks.map((task, i) => (
<TaskItem key={`${task}_${i}`} task={task} onDelete={onDelete} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly this is the same even if we remove the key prop. Needs an alternative "id generation"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think just using a timestamp here as an "id" is sufficient?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need something like

interface Task {
  id: string;
  text: string;
}

and then use id as the key.

Copy link
Contributor Author

@theborowski theborowski Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored this to use an object as you suggested. This fixes the key problem as well as improves deletion with using the id of the task. I also moved the task creation in a function so that clicking and pressing enter have the same behavior. Added a little clip of the app working with these changes in the PR description.

@theborowski theborowski requested a review from lxsmnsyc December 18, 2024 03:08
@RobPruzan RobPruzan force-pushed the main branch 2 times, most recently from 7526d13 to 47c46ca Compare December 21, 2024 01:52
@theborowski theborowski force-pushed the chore/update-kitchen-sink branch from 174c4b2 to 9f67524 Compare January 3, 2025 16:39
@theborowski theborowski force-pushed the chore/update-kitchen-sink branch from 9f67524 to 88c13db Compare January 3, 2025 16:42
@theborowski
Copy link
Contributor Author

@lxsmnsyc are there any additional changes needed in order to get this merged in?

@lxsmnsyc
Copy link
Collaborator

lxsmnsyc commented Jan 4, 2025

Nothing specifically. LGTM

Copy link

pkg-pr-new bot commented Jan 4, 2025

Open in Stackblitz

npm i https://pkg.pr.new/aidenybai/react-scan@136

commit: 88c13db

@lxsmnsyc lxsmnsyc merged commit 7a57c11 into aidenybai:main Jan 4, 2025
2 of 3 checks passed
@theborowski theborowski deleted the chore/update-kitchen-sink branch January 4, 2025 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants